delegation: fix cycles during delayed lowering#154368
delegation: fix cycles during delayed lowering#154368aerooneqq wants to merge 7 commits intorust-lang:mainfrom
Conversation
|
What specific uses of |
|
This should really have a reviewer with a lot of query system experience. r? @Zoxc |
|
Failed to set assignee to
|
This comment has been minimized.
This comment has been minimized.
f32739b to
f50159f
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
f50159f to
5afd6e4
Compare
|
So the reason of this cycle was that this function wants to iterate over all free items, however they were not available during delayed lowering as it was executed before rust/compiler/rustc_middle/src/ty/print/pretty.rs Line 3393 in e3691a5 Next, there is one more problem in this function, which is maybe not that actual now, but it will be during supporting inherent impls, as during function resolution we will do coherence check, and in cases like: struct X<T> { t: T }
// No generics provided
impl X {
pub fn foo() {}
}
reuse X::foo;we would still come to rust/compiler/rustc_middle/src/ty/print/pretty.rs Line 3398 in e3691a5 @rustbot ready |
This comment has been minimized.
This comment has been minimized.
| node => node.ident(), | ||
| } | ||
| pub fn hir_opt_ident(self, id: HirId) -> Option<Ident> { | ||
| self.hir_crate(()).opt_ident(self, id) |
There was a problem hiding this comment.
This use of hir_crate looks like it may be a source of perf regressions in incremental mode, I'll run benchmarks.
compiler/rustc_middle/src/hir/map.rs
Outdated
| }; | ||
|
|
||
| let delayed_kinds = tcx.hir_crate(()).delayed_owners_kinds(); | ||
| let delayed_kinds = delayed_kinds.filter(|(id, _)| match collection_kind { |
There was a problem hiding this comment.
So calls like tcx.hir_module_items(specific_module) now need to walk the whole crate instead of just specific_module.
Ideally, delayed_ids should perhaps be per-module to fit best into the current setup.
(Not sure if this needs to be addressed right now, perhaps we'll see after running benchmarks.)
|
@rustbot ready |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…<try> delegation: fix cycles during delayed lowering
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (40807f4): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 6.6%, secondary 2.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 13.2%, secondary 3.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 483.294s -> 485.596s (0.48%) |
|
regressions are mostly in |
|
☔ The latest upstream changes (presumably #154503) made this pull request unmergeable. Please resolve the merge conflicts. |
View all comments
This PR forces lowering of delayed owners after
hir_crate_items, as some diagnostics usehir_crate_itemswhich results in query cycle which is then hangs callingdef_path_stragain and again. Fixes #154169. Part of #118212.r? @petrochenkov